Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process: ensure message sanity at send source #24787

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

gireeshpunathil
Copy link
Member

Error messages coming out of de-serialization at the send target is not consumable. So detect the scenario and fix it at the send source.

Ref: #20314

existing error message (sample code in the linked issue):

SUBPROCESS error: SyntaxError: Unexpected token u in JSON at position 0
    at JSON.parse (<anonymous>)
    at Pipe.channel.onread (internal/child_process.js:492:28)

with this change:

PARENT error: TypeError [ERR_INVALID_ARG_TYPE]: The "message" argument must be of type serializable objects. Received type symbol
    at ChildProcess.target._send (internal/child_process.js:664:13)
    at ChildProcess.target.send (internal/child_process.js:641:19)
    at Object.<anonymous> (/home/gireesh/node/parent.js:6:14)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:734:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:776:12)
    at executeUserCode (internal/bootstrap/node.js:343:17)

/cc @vsemozhetbyt @joyeecheung @nodejs/child_process

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Dec 2, 2018
lib/internal/child_process.js Outdated Show resolved Hide resolved
// end point; as any failure in the strinfication there
// will result in error message that is weekly consumable.
// So perform a sanity check on message prior to sending.
const ret = JSON.stringify(message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling JSON.stringify() twice is wasteful. Cache the result and use it on line 730.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately, the message undergoes change in all the possible control flows between here and line 730, so cached value is unusable.

Another alternative is to delay this sanity check to as late as line 730 where there is already a stringification, don't know whether it can have side effects or not - thoughts please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only guard against three input types: undefined, function and symbol. All other values either throw (e.g., any input that includes a BigInt) or the non-serializable part would be omitted or replaced with the string 'null' (with the exception of the object containing an toJSON function which returns undefined).

There are lots of inputs which would also be problematic which would not be caught here. I actually doubt that we really have to guard against anything else than undefined. So let's just use a simple type check instead of using JSON.stringify:

if (typeof message !== 'string' &&
    typeof message !== 'object' &&
    typeof message !== 'number' &&
    typeof message !== 'boolean') {
  throw new ERR_INVALID_ARG_TYPE(
    'message', ['string', 'object', 'number', 'boolean'], message);
}

The reason why I would do the check like this is that in the default case the message is either a string or an object. Other type checks would not be necessary anymore. While doing it the other way around would require to always check for undefined, bigint, symbol and function. The error message is also a bit clearer that way.

This will not guard against a weird toJSON function but that should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BridgeAR - I followed your suggestions, PTAL.

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil
Copy link
Member Author

more than a month with no reviews and approvals; can I have one please!

// end point; as any failure in the strinfication there
// will result in error message that is weekly consumable.
// So perform a sanity check on message prior to sending.
const ret = JSON.stringify(message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only guard against three input types: undefined, function and symbol. All other values either throw (e.g., any input that includes a BigInt) or the non-serializable part would be omitted or replaced with the string 'null' (with the exception of the object containing an toJSON function which returns undefined).

There are lots of inputs which would also be problematic which would not be caught here. I actually doubt that we really have to guard against anything else than undefined. So let's just use a simple type check instead of using JSON.stringify:

if (typeof message !== 'string' &&
    typeof message !== 'object' &&
    typeof message !== 'number' &&
    typeof message !== 'boolean') {
  throw new ERR_INVALID_ARG_TYPE(
    'message', ['string', 'object', 'number', 'boolean'], message);
}

The reason why I would do the check like this is that in the default case the message is either a string or an object. Other type checks would not be necessary anymore. While doing it the other way around would require to always check for undefined, bigint, symbol and function. The error message is also a bit clearer that way.

This will not guard against a weird toJSON function but that should be fine.

// will result in error message that is weakly consumable.
// So perform a sanity check on message prior to sending.
const ret = JSON.stringify(message);
if (ret == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ret can never be null. If null is passed through as message JSON.stringify() would return the string 'null'.

JSON.stringify() either returns undefined or a string or it throws.

@gireeshpunathil
Copy link
Member Author

ping @BridgeAR

@gireeshpunathil
Copy link
Member Author

can I get a review here pls?

@lpinca
Copy link
Member

lpinca commented Mar 11, 2019

Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: nodejs#20314

PR-URL: nodejs#24787
Reviewed-By: Luigi Pinca <[email protected]>
@gireeshpunathil
Copy link
Member Author

landed as daa97df

@gireeshpunathil gireeshpunathil merged commit daa97df into nodejs:master Mar 18, 2019
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: nodejs#20314

PR-URL: nodejs#24787
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Mar 27, 2019
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: #20314

PR-URL: #24787
Reviewed-By: Luigi Pinca <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
Error messages coming out of de-serialization at the send target
is not consumable. So detect the scenario and fix it at the send source

Ref: #20314

PR-URL: #24787
Reviewed-By: Luigi Pinca <[email protected]>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants